/hold

This has a bit more stuff in it than it should.

/hold cancel

Removed the extraneous stuff.

#2645

even previously the cache was uncompressing the data, although implicitly. why not update the cache to make sure it uncompresses the image always..?

This is not ideal as every comsumer needs to decompress when the cache should do it as expected.

even previously the cache was uncompressing the data, although implicitly. why not update the cache to make sure it uncompresses the image always..?

This is not ideal as every comsumer needs to decompress when the cache should do it as expected.

+1

This also affects the baremetal IPI platform as we use the QEMU image for our bootstrap:

We will also need to have a change there to fix it.

It would be nice if the cache just held the uncompressed image already, can any platform take the gzipped? libvirt can't, and it appears openstack can't... so is there a reason to keep it around?

Does anyone know why we're gzipping a qcow image, instead of making sure the image itself is fully compressed and sparisfied?

Looks good, but I agree we need a solution for IPI baremetal and it would be nice if the decompression logic could be abstracted behind the existing cache interfaces so each platform doesn't have to care explicitly about it.

Does anyone know why we're gzipping a qcow image, instead of making sure the image itself is fully compressed and sparisfied?

I think @yuqi-zhang and @miabbott have the history here.

Does anyone know why we're gzipping a qcow image, instead of making sure the image itself is fully compressed and sparisfied?

I think @yuqi-zhang and @miabbott have the history here.

I can weigh in here. We've always gzip'd the images but previously we did not have the .gz extension. Even though the documentation told folks the images were gz and would be uncompressed via IPI download or the specific provided curl command, many folks manually downloaded, used the images as is, and they failed as the image wasn't valid without decompressing. To make it more obvious for everyone we added the .gz extension.

To add a bit, the coreos-assembler generates a qcow image from a base ostree, which is the base for all other images. These are then all zipped in the RHCOS pipeline before uploading.
We gzip e.g. the metal image because the coreos-installer takes a gzipped image for bare-metal installations. This has been the case since 4.1.

If you feel that the compression schema should be changed, that should probably come as an RFE since we haven't really explored it.

even previously the cache was uncompressing the data, although implicitly. why not update the cache to make sure it uncompresses the image always..?
This is not ideal as every comsumer needs to decompress when the cache should do it as expected.

+1

This also affects the baremetal IPI platform as we use the QEMU image for our bootstrap:

installer/pkg/tfvars/baremetal/baremetal.go

Line 34 in a498f63

bootstrapOSImage, err := libvirttfvars.CachedImage(bootstrapOSImage)

We will also need to have a change there to fix it.

It would be nice if the cache just held the uncompressed image already, can any platform take the gzipped? libvirt can't, and it appears openstack can't... so is there a reason to keep it around?

I agree, doing this in the cache makes the most sense.

fedora coreos is using xz https://builds.coreos.fedoraproject.org/streams/testing.json
cc @ashcrow @yuqi-zhang
So this special casing to gz won't translate well soon..

And therefore i think i would like to keep the uncompressing part hidden into the downloading-caching code.

We've always gzip'd the images but previously we did not have the .gz extension. Even though the documentation told folks the images were gz and would be uncompressed via IPI download or the specific provided curl command, many folks manually downloaded, used the images as is, and they failed as the image wasn't valid without decompressing.

That hasn't been our experience for IPI baremetal FWIW - we've been downloading the openstack qcow2 for several months directly, and not gunzipped it until now:

$ file rhcos-43.81.20191028.2-openstack.x86_64.qcow2/rhcos-43.81.20191028.2-openstack.x86_64.qcow2
rhcos-43.81.20191028.2-openstack.x86_64.qcow2/rhcos-43.81.20191028.2-openstack.x86_64.qcow2: QEMU QCOW Image (v3), 17179869184 bytes

We had to explicitly unzip it ref openshift/ironic-rhcos-downloader#12 and also it seems the HTTP headers provided changed as well ref openshift/ironic-rhcos-downloader#13

fedora coreos is using xz https://builds.coreos.fedoraproject.org/streams/testing.json
cc @ashcrow @yuqi-zhang
So this special casing to gz won't translate well soon..

And therefore i think i would like to keep the uncompressing part hidden into the downloading-caching code.

I agree and I'm working on that.

Latest commit does decompression transparently in the cache (inspecting the file if it already exists). This simplifies everything.

@abhinavdahiya please re-review, after which I'll collapse the changes.

@abhinavdahiya rdy;

looks good @RobertKrawitz I think we can squash and move ahead.

@RobertKrawitz: This pull request references Bugzilla bug 1772599, which is valid.

/retest
govet failure was running out of memory.

/hold cancel

@stbenjam @abhinavdahiya please re-review and lgtm if appropriate.

/approve

@stbenjam (or someone) could you /lgtm please?

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, cgwalters, hexfusion, RobertKrawitz, stbenjam

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/retest

/retest

Please review the full test history for this PR and help us cut down flakes.

I think previously the Golang HTTP stack transparently did the equivalent of curl's --compressed. So yes, it Just Worked before, and the switch here definitely requires installer changes.

This accords with normal practice. The client normally decodes the transferred object in accord with the content-transfer-encoding:. There's a world of difference between content-transfer-encoding: gzip with content-type: binary/octet-stream and content-transfer-encoding: 8bit with content-type: application/x-gzip, even if the data stream on the wire is identical.

@RobertKrawitz: All pull requests linked via external trackers have merged. Bugzilla bug 1772599 has been moved to the MODIFIED state.